Skip to content

rustc: use more correct span data in for loop desugaring #88214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 11, 2021
Merged

rustc: use more correct span data in for loop desugaring #88214

merged 2 commits into from
Sep 11, 2021

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 21, 2021 •
edited
Loading

Fixes #82462

Before:

  help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
     |
  LL |     for x in DroppingSlice(&*v).iter(); {
     |                                       +

After:

  help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
     |
  LL |     };
     |      +

This seems like a reasonable fix: since the desugared "expr_drop_temps_mut" contains the entire desugared loop construct, its span should contain the entire loop construct as well.

@rust-highfive
Copy link
Contributor

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle notriddle force-pushed the notriddle/for-loop-span-drop-temps-mut branch from 31d6b0a to d138326 Compare August 21, 2021 20:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa
Copy link
Member

nagisa commented Aug 22, 2021

On one hand this looks like a nice improvement in that we no longer suggest non-sense, but on the other adding a semicolon there won't help either way (and the message sounds quite confident it would help…)

Please squash. r=me.

@notriddle notriddle force-pushed the notriddle/for-loop-span-drop-temps-mut branch from e665aa4 to 4b3bcb7 Compare August 23, 2021 00:57
@notriddle
Copy link
Contributor Author

@nagisa okay, it's squashed

@nagisa
Copy link
Member

nagisa commented Aug 23, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 23, 2021

📌 Commit 4b3bcb72a81c3bc8519366d071dc9a06e8ef2174 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2021
@bors
Copy link
Collaborator

bors commented Aug 23, 2021

⌛ Testing commit 4b3bcb72a81c3bc8519366d071dc9a06e8ef2174 with merge 0edc41c7315527e739aca1cb018d2459dee61abe...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Aug 23, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 23, 2021
@notriddle notriddle force-pushed the notriddle/for-loop-span-drop-temps-mut branch from 4b3bcb7 to b0a9a42 Compare August 23, 2021 14:51
@notriddle
Copy link
Contributor Author

notriddle commented Aug 23, 2021 •
edited
Loading

I guess I'll rebase onto the master branch? I'm not actually sure what's wrong here: after locally rebasing and running with --bless, it didn't change the output.

@nagisa
Copy link
Member

nagisa commented Aug 23, 2021

Did you re-generate the .stderr file after the rebase? Bors will always merge in master when testing a diff and this is the diff from the test output, while the PRd stderr file still contains the larger span.

---- [ui (nll)] ui/borrowck/issue-82462.rs stdout ----
diff of stderr:

7	   |              |               immutable borrow occurs here
8	   |              a temporary with access to the immutable borrow is created here ...
9	LL |         v.push(*x);
-	   |         ^^^^^^^^^^ mutable borrow occurs here
+	   |         ^ mutable borrow occurs here
11	LL |         break;
12	LL |     }
13	   |     - ... and the immutable borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `DroppingSlice`

@notriddle notriddle force-pushed the notriddle/for-loop-span-drop-temps-mut branch from b0a9a42 to 0b451b8 Compare August 23, 2021 23:56
@notriddle
Copy link
Contributor Author

Yes, I tried to regenerate it and nothing happened.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@nagisa
Copy link
Member

nagisa commented Aug 26, 2021

@bors r+

Lets try it again, then.

@bors
Copy link
Collaborator

bors commented Aug 26, 2021

📌 Commit 0b451b8c57fa6fdbe6ef460a1362bf1cfed70d5d has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2021
@nagisa
Copy link
Member

nagisa commented Aug 26, 2021

@bors rollup=never

@notriddle
Copy link
Contributor Author

Okay, so it's not my local install. Why are optimizations impacting the format of error output?

@nagisa
Copy link
Member

nagisa commented Aug 28, 2021 •
edited
Loading

Hm, since this error is generated by borrowck, it could be that we inadvertently run some MIR pass before borrowck that we don't run in non-optimized builds? (You could verify with -Zmir-opt-level or whatever the flag is called)

@notriddle
rustc: use more correct span data in for loop desugaring
fe1a7f7 
Before:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     for x in DroppingSlice(&*v).iter(); {
         |                                       +

After:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     };
         |      +

This seems like a reasonable fix: since the desugared "expr_drop_temps_mut"
contains the entire desugared loop construct, its span should contain the
entire loop construct as well.
@notriddle
Copy link
Contributor Author

Found it. It's actually caused by test-compare-mode, and not the optimization stack.

I've added a second stderr file for this.

@notriddle notriddle force-pushed the notriddle/for-loop-span-drop-temps-mut branch from 579e937 to fe1a7f7 Compare August 28, 2021 19:19
@nagisa
Copy link
Member

nagisa commented Sep 5, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 5, 2021

📌 Commit fe1a7f7 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2021
@bors
Copy link
Collaborator

bors commented Sep 5, 2021

⌛ Testing commit fe1a7f7 with merge f6f0cf7d2042b00d9eb2bf23a3c4b89afd3e7073...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Sep 5, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 5, 2021
@notriddle
Copy link
Contributor Author

@nagisa New commit to fix the broken Clippy lints.

@nagisa
Copy link
Member

nagisa commented Sep 10, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 10, 2021

📌 Commit 543e601 has been approved by nagisa

@bors
Copy link
Collaborator

bors commented Sep 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2021
@bors
Copy link
Collaborator

bors commented Sep 11, 2021

⌛ Testing commit 543e601 with merge 4e880f8...

@bors
Copy link
Collaborator

bors commented Sep 11, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 4e880f8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2021
@bors bors merged commit 4e880f8 into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e880f8): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@notriddle notriddle deleted the notriddle/for-loop-span-drop-temps-mut branch September 28, 2021 06:21
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2021
@bors
Auto merge of rust-lang#88214 - notriddle:notriddle/for-loop-span-dro…
eac0b26 
…p-temps-mut, r=nagisa

rustc: use more correct span data in for loop desugaring

Fixes rust-lang#82462

Before:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     for x in DroppingSlice(&*v).iter(); {
         |                                       +

After:

      help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
         |
      LL |     };
         |      +

This seems like a reasonable fix: since the desugared "expr_drop_temps_mut" contains the entire desugared loop construct, its span should contain the entire loop construct as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nagisa nagisa

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

Diagnostic for iterator referencing temporary with Drop may suggest an invalid semicolon
8 participants
@notriddle @rust-highfive @rust-log-analyzer @nagisa @bors @rust-timer @inquisitivecrystal @rustbot